-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Clean warnings when all warning enabled #2112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Not used variables / functions due to debug log Dual define with different values : cores\esp32/binary.h #define B110 6 #define B1000000 64 tools/sdk/include/newlib/sys/termios.h #define B110 3 #define B1000000 23 Local variable returned in WiFiclient Secure
libraries/WiFi/src/WiFiClient.cpp
Outdated
@@ -439,6 +439,9 @@ uint8_t WiFiClient::connected() | |||
if (_connected) { | |||
uint8_t dummy; | |||
int res = recv(fd(), &dummy, 0, MSG_DONTWAIT); | |||
if(res < 0) { | |||
log_e("%d", errno); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging the errno is not meaningful without a message to say why it is being logged. It would also be beneficial to log the "res" value here possibly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right should be res and not errno - will update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to : log_e("RES: %d", res);
@@ -306,9 +307,12 @@ static void i2cDumpDqData(i2c_t * i2c) | |||
} | |||
a++; | |||
} | |||
#else | |||
log_i("Debug Buffer not Enabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be better to not log anything at all here so as not to clutter the console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except, if debug level is at "info" I assume you actually want to see information?
The debug buffer in hal-i2c takes up 2570 bytes of ram if enabled.
That is why this message exists. If you set debug_level at or above "info" I expect you have a problem you are trying to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stickbreaker this function isn't called from what I can tell if the level isn't at least DEBUG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to follow same as others functions - should I change it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I got confused with the add/subtract highlighting. I just though you were questioning the "enable" message. My only excuse is that I hadn't had my coffee before I read your Post 😃
I don't have any problems with your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other question on the debug subject: I added a an app level control for debug output with Wire.setDebugFlags( uint32_t setBits, uint32_t resetBits);
, but I did not document how/when to use it. Is there any standard for such documentation?
I used it around calls to a "recalcitrant" i2c devices when I get unexpected responses. I don't know if it would be good to put "debug" conditionals around all levels of this code (i.e. Wire() and hal-i2c).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stickbreaker @atanisoft I followed same as line 354 - it means, I need to remove original 354 as well ? Or I misunderstood ?
thanks for your review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luc-github Let me spend some time looking through it. The "debug_buffer" only houses interrupt activity(used inside the ISR). I just scanned through i2cDumpDqData()
and don't know why I had it conditional on "debug_buffer"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stickbreaker thank you - I did not wanted to break anything that is why I have mimic existing code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason github won't let me create an edited version of your version? So, I'm going to create an edited version the current espressif version in my repo? (might work? very confusing to me anyway!)
libraries/WiFi/src/WiFiMulti.cpp
Outdated
mac = WiFi.BSSID(); | ||
{ | ||
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG | ||
IPAddress ip = WiFi.localIP(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can not declare new variables inside the case. Add another #if
for their definitions above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the variable is not necessary at all even, the log_d line can be updated to just:
log_d("[WIFI] IP: %s", WiFi.localIP().toString().c_str());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for BSSID probably with WiFi.BSSIDstr()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is not a bad idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will do
For my personnal knowledge - as I have set brace, why it is not correct to declare new local variables inside the case ?
@@ -683,7 +683,9 @@ void WiFiSTAClass::_smartConfigCallback(uint32_t st, void* result) { | |||
smartconfig_status_t status = (smartconfig_status_t) st; | |||
log_d("Status: %s", sc_status_strings[st % 5]); | |||
if (status == SC_STATUS_GETTING_SSID_PSWD) { | |||
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG | |||
smartconfig_type_t * type = (smartconfig_type_t *)result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should also include the call to log_d in the #if
. Same for the one below. Makes the reason for the #if
clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
@@ -232,18 +232,17 @@ bool WiFiClientSecure::verify(const char* fp, const char* domain_name) | |||
} | |||
|
|||
char *WiFiClientSecure::_streamLoad(Stream& stream, size_t size) { | |||
char *dest = (char*)malloc(size); | |||
static char *dest = nullptr; | |||
if(dest)free(dest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D Please at least try to put some style.
if(dest) {
free(dest);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
libraries/WiFi/src/WiFiSTA.cpp
Outdated
ip4_addr_t * ip = (ip4_addr_t *)result; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the log_d line inside the if/endif block as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
char *dest = (char*)malloc(size); | ||
static char *dest = nullptr; | ||
if(dest)free(dest); | ||
dest = (char*)malloc(size); | ||
if (!dest) { | ||
return nullptr; | ||
} | ||
if (size != stream.readBytes(dest, size)) { | ||
free(dest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set dest to null here, or free will be called again on this region and bad things might happen :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes correct
tools/sdk/include/vfs/esp_vfs.h
Outdated
@@ -27,7 +27,7 @@ | |||
#include <sys/reent.h> | |||
#include <sys/stat.h> | |||
#include <sys/time.h> | |||
#include <sys/termios.h> | |||
//#include <sys/termios.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comes from ESP-IDF and should be fixed on that side instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that was my problem as in esp_arduino it is the solution in current context - but in IDF it may be necessary , I do not use idf and this change could be rejected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some things here and there :P please take care :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the requested changes - including putting back :
#include <sys/termios.h>
But requested this change in IDF has big chance to be rejected, No ?
Or any other solution ? for :
Dual define with different values :
cores\esp32/binary.h
#define B110 6
#define B1000000 64
tools/sdk/include/newlib/sys/termios.h
#define B110 3
#define B1000000 23
is binary.h a must ?
libraries/WiFi/src/WiFiSTA.cpp
Outdated
ip4_addr_t * ip = (ip4_addr_t *)result; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
char *dest = (char*)malloc(size); | ||
static char *dest = nullptr; | ||
if(dest)free(dest); | ||
dest = (char*)malloc(size); | ||
if (!dest) { | ||
return nullptr; | ||
} | ||
if (size != stream.readBytes(dest, size)) { | ||
free(dest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes correct
tools/sdk/include/vfs/esp_vfs.h
Outdated
@@ -27,7 +27,7 @@ | |||
#include <sys/reent.h> | |||
#include <sys/stat.h> | |||
#include <sys/time.h> | |||
#include <sys/termios.h> | |||
//#include <sys/termios.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that was my problem as in esp_arduino it is the solution in current context - but in IDF it may be necessary , I do not use idf and this change could be rejected
libraries/WiFi/src/WiFiMulti.cpp
Outdated
mac = WiFi.BSSID(); | ||
{ | ||
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG | ||
IPAddress ip = WiFi.localIP(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will do
For my personnal knowledge - as I have set brace, why it is not correct to declare new local variables inside the case ?
we can not touch sdk files :) the binary strings are still used here and there in libs :( if we get rid of them I expect other issues to come |
so is this ready to be merged now? all i2c things resolved? |
No I still wait for @stickbreaker What could be the workaround for #define conflict - it is source of potential issue also I think |
where is esp_vfs included that it causes this issue? |
@luc-github keep the debug_enable conjunction, that way just setting debug level doesn't cause serial monitor spamming. The two step process, hopefully we wont use i2c debugging constantly. I had forgotten my reasoning for the double level activation. Chuck |
@luc-github in this case you undefine the offending arduino bin definitions before you include the file. That file does not care for Arduino binary definitions ;) |
so we left as it is ? as a known issue ?If it is agreed I am fine with it ^_^ - so PR is done
|
@luc-github what will happen if before https://github.com/espressif/arduino-esp32/blob/master/libraries/SD/src/sd_diskio.cpp#L19 you add |
is esp_vfs.h even used? from a cursory glance at sd_diskio.cpp it doesn't appear to be |
@me-no-dev seems avoid issue
@atanisoft seems not as commented compilation succeed and no warning
@me-no-dev @atanisoft I think commenting |
sure! go ahead :) |
I think everything is clear now - thank you guys ^_^ |
Not used variables / functions due to debug log
Dual define with different values :
cores\esp32/binary.h
#define B110 6
#define B1000000 64
tools/sdk/include/newlib/sys/termios.h
#define B110 3
#define B1000000 23
Local variable returned in WiFiclient Secure